Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs: check subdir correctly in cpSync #55033

Merged
merged 1 commit into from
Sep 28, 2024
Merged

fs: check subdir correctly in cpSync #55033

merged 1 commit into from
Sep 28, 2024
+46 −4

Conversation

jazelly
Copy link
Member

@jazelly jazelly commented Sep 20, 2024

Fixes: #54285

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Sep 20, 2024
@jazelly jazelly changed the title fs: check subdir correct in cpSync fs: check subdir correctly in cpSync Sep 20, 2024
@jazelly jazelly marked this pull request as ready for review September 20, 2024 21:43
@jazelly
Copy link
Member Author

jazelly commented Sep 20, 2024 •
edited
Loading

This probably will have merge conflicts with #54653. I'd like the other one to land first as this one is easier to rebase, re-review and re-approve, if that sounds good : )

@jazelly jazelly marked this pull request as draft September 20, 2024 21:57
@RedYetiDev RedYetiDev added the blocked PRs that are blocked by other issues or PRs. label Sep 20, 2024
@jazelly jazelly marked this pull request as ready for review September 20, 2024 22:05
@RedYetiDev
Copy link
Member

Per the comment above, I've added blocked, however, feel free to adjust.

@codecov Codecov
Copy link

codecov bot commented Sep 20, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.23%. Comparing base (cfe58cf) to head (9358d9e).
Report is 75 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #55033   +/-   ##
=======================================
  Coverage   88.23%   88.23%           
=======================================
  Files         652      652           
  Lines      183912   183916    +4     
  Branches    35865    35862    -3     
=======================================
+ Hits       162272   162283   +11     
+ Misses      14915    14910    -5     
+ Partials     6725     6723    -2
Files with missing lines Coverage Δ
src/node_file.cc 77.06% <80.00%> (-0.01%) ⬇️

... and 33 files with indirect coverage changes

@jakecastelli
Copy link
Member

cc. @nodejs/fs @anonrig @aduh95 (surprised that both of you aren't in the fs team 😄)

jakecastelli
jakecastelli reviewed Sep 22, 2024
View reviewed changes
Copy link
Member

@jakecastelli jakecastelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other PR has landed and indeed there are merge conflicts now.

@jakecastelli jakecastelli removed the blocked PRs that are blocked by other issues or PRs. label Sep 22, 2024
@jazelly
Copy link
Member Author

jazelly commented Sep 22, 2024

Conflicts resolved ^ PTAL

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62685/

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 24, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 24, 2024
@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 24, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/55033
✔  Done loading data for nodejs/node/pull/55033
----------------------------------- PR info ------------------------------------
Title      fs: check subdir correctly in cpSync (#55033)
Author     Jason Zhang <xzha4350@gmail.com> (@jazelly)
Branch     jazelly:fix-54285 -> nodejs:main
Labels     c++, fs, author ready, needs-ci
Commits    1
 - fs: check subdir correctly in cpSync
Committers 1
 - Jason Zhang <xzha4350@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/55033
Fixes: https://github.com/nodejs/node/issues/54285
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55033
Fixes: https://github.com/nodejs/node/issues/54285
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 20 Sep 2024 21:03:08 GMT
   ✔  Approvals: 1
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/55033#pullrequestreview-2321643883
   ✘  This PR needs to wait 71 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-09-23T09:40:40Z: https://ci.nodejs.org/job/node-test-pull-request/62685/
- Querying data for job/node-test-pull-request/62685/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/11021901134

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62808/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62820/

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Sep 28, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 28, 2024
@nodejs-github-bot nodejs-github-bot merged commit 7fe5bcd into nodejs:main Sep 28, 2024
69 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 7fe5bcd

targos pushed a commit that referenced this pull request Oct 4, 2024
@jazelly @targos
fs: check subdir correctly in cpSync
a43ce87 
PR-URL: #55033
Fixes: #54285
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Oct 4, 2024
@jazelly @targos
fs: check subdir correctly in cpSync
1c94725 
PR-URL: #55033
Fixes: #54285
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@aduh95 aduh95 mentioned this pull request Oct 9, 2024
Merged
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@jazelly @louwers
fs: check subdir correctly in cpSync
8279f0c 
PR-URL: nodejs#55033
Fixes: nodejs#54285
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakecastelli jakecastelli jakecastelli left review comments

@RedYetiDev RedYetiDev RedYetiDev left review comments

@aduh95 aduh95 aduh95 approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fs.cpSync fails when copying to a directory with a name starting with the source directory name
6 participants
@jazelly @RedYetiDev @jakecastelli @nodejs-github-bot @joyeecheung @aduh95